@pitkling added some fair criticisms of this approach here

Continuing the discussion from #249364 (especially this comment) here, to avoid spamming that issue unnecessarily.

First of all, thanks for digging further into this approach! It's definitely worth pursuing and might be a good alternative to PR #328890.

From your issue description, I get it that the CAP_DAC_OVERRIDE requirement of espanso is a wayland-only thing? Is that documented somewhere? Just asking, since in that case I'll update my draft PR #328890 accordingly.

(quotes from #249364, this comment)

[…]

This means that the espanso team could remove/change/substitute/rename/… that subcommand any time

True, but the nix input won't change until we change it. I've been trying to help the espanso team here and there; there's been talk about a big-ish refactor of the GUI to remove the wx dependency, but I haven't heard of any immediate plans for a major refactoring of the backend. (EDIT: espanso roadmap)

True enough, there won't be problems until we update the derivation. I'm mostly worried about upstream changes in the backend that we are unaware of and that do not cause build problems. E.g., the espanso team might keep the worker subcommand but add additional features relying on some monitor process managing the worker. We would not spot those problems on a derivation update but they could cause problems much later that might be difficult to figure out.

automatically restarting the worker if the configuration changed

Good point, I need to double check this. Requiring manual reload to pick up config changes would be a bit of a bummer (since they've already done the work to make this smoother).

The systemd unit should be automatically restarting on failure, which might obviate the need for some of their checks.

Good point, that definitely helps. Still, there must be more to it, right? Otherwise, why would the espanso team not make use of this themselves in their reference systemd service?

Even if it works now, it might break future features if some internals change.

That's of course possible, though I'm not sure what approach could prevent upstream changes from breaking the always-unconventional nix way. […]

That's true, it's not possible to avoid such issues. I'm just worried that this approach is more prone to these problems since (a) it relies on the seemingly not-well documented workings of an internal subcommand and (b) we might not even spot problems until much later when updating the derivation (as I mentioned above).

It seems to me that the approach via the wrapper library from #328890 (which definitely has its own problems!) has the benefit on not relying on internals (at least not much; I don't think it's likely that the espanso team changes process forking in a way that is not captures by the wrapper library) and (b) if there are upstream changes breaking that approach, we should realize this very quickly (not necessarily at build time, but the process will most likely not work at all, as it is currently the case at least on wayland).

Btw., I will sometime soon (currently somewhat busy) create a issue for discussing whether such problems can be handled directly by security.wrappers, as you suggested here. But I think we should implement a fix for #249364 (whether this PR or #328890) anyways, since I think even if security.wrappers can handle this, it will take some time to implement such a change.

From your issue description, I get it that the CAP_DAC_OVERRIDE requirement of espanso is a wayland-only thing? Is that documented somewhere? Just asking, since in that case I'll update my draft PR #328890 accordingly.

I think so -- I linked to the linux installation instructions above, capabilities are only mentioned in the wayland section.

I'm mostly worried about upstream changes in the backend that we are unaware of and that do not cause build problems.

Certainly possible, but I think you might be over-worried here. I am involved in espanso development and intend to become more involved over time. Similarly, I'm listed as a maintainer of the espanso package and module; with any luck I'll be able to help watch for issues here going forward.

Can you elaborate on a worst-case scenario in your mind? I want to make sure we're speaking the same language; in my mind, you're suggesting that:

1. an upstream minor or patch-level change could reasonably change the behavior of espanso worker, since it's not part of their public API
2. if this were the case, we might update the src and do some basic testing* and find that espanso still seems to work and update the derivation
3. at some point, someone would discover an edge case in which something wasn't working and file a bug report
4. in that case, we would likely have to revert the PR updating src -- fairly trivial to do in nixos -- and then consider other options (perhaps security.wrappers will have an option to intercept /proc/self/exe calls by then).

Is that more-or-less the risks that concern you? As a hobbyist self-taught programmer, sometimes I am completely blind to the concerns that professionals assume I am taking into account.

* perhaps we could consider adding a nixos test for espanso?

Still, there must be more to it, right? Otherwise, why would the espanso team not make use of this themselves in their reference systemd service?

I might be misunderstanding -- their reference service does leverage systemd's automatic service restarts: Restart=on-failure, which is what I was referring to.

Also -- for context -- espanso's creator and primary dev @federico-terzi has done an amazing job but has openly noted that he is not going to be able to focus on espanso for the time being (espanso/espanso#1742); @AucaCoyan and others have stepped up, but issues like espanso/espanso#1953, espanso/espanso#1963, and many others show that there is still a lot of work to be done to even get development back on track. I only bring this up to point out that they are an extremely welcoming community that is hungry for additional contributors; I still think that an upstream patch should be considered if we can come up with a strategy that would ameliorate nix issues and be a win (or net even) for the espanso team. They've showed a lot of flexibility working with 3rd party package management in the past (espanso/espanso#615) and are generally very nice people.

From your issue description, I get it that the CAP_DAC_OVERRIDE requirement of espanso is a wayland-only thing? Is that documented somewhere? Just asking, since in that case I'll update my draft PR #328890 accordingly.

I think so -- I linked to the linux installation instructions above, capabilities are only mentioned in the wayland section.

Ahh, you're right. I'll update my PR draft accordingly.

I'm mostly worried about upstream changes in the backend that we are unaware of and that do not cause build problems.

[..] Can you elaborate on a worst-case scenario in your mind? I want to make sure we're speaking the same language; in my mind, you're suggesting that:

1. an upstream minor or patch-level change could reasonably change the behavior of espanso worker, since it's not part of their public API
2. if this were the case, we might update the src and do some basic testing* and find that espanso still seems to work and update the derivation
3. at some point, someone would discover an edge case in which something wasn't working and file a bug report
4. in that case, we would likely have to revert the PR updating src -- fairly trivial to do in nixos -- and then consider other options (perhaps security.wrappers will have an option to intercept /proc/self/exe calls by then).

Is that more-or-less the risks that concern you? […]

Yes, that's basically it. In general I think it's more robust to stick to openly documented interfaces, to keep maintenance low. That's also why I still think the approach from #328890 requires probably lower maintenance of the NixOS module code (major concern for #328890) than the approach here. But given that you're involved in espanso's development definitely weakens those arguments.

Still, there must be more to it, right? Otherwise, why would the espanso team not make use of this themselves in their reference systemd service?

I might be misunderstanding -- their reference service does leverage systemd's automatic service restarts: Restart=on-failure, which is what I was referring to. […]

Yes, but they're running espanso launcher, not espanso worker. I just wanted to point out that this is another indicator that the launcher does more than simply restarting espanso on failures.

Ahh, you're right. I'll update my PR draft accordingly.

Trying to test to confirm (on an Arch host running i3 over VNC, with espanso built from source), espanso runs and seems to try to expand without any additional capabilities. Unfortunately it succeeds at deleting the trigger text but the replacement text doesn't appear; I suspect this is an issue with the clipboard and VNC. It is definitely different than the "crash at launch" that I get on Wayland when trying to run without capabilities.

Also, regarding your PR -- IMO users installing espanso with package = pkgs.espanso-wayland should -- with documentation -- be opting into the wrapper by default. If espanso cannot work on wayland without these capabilities, and we're not going to provide a service that runs as root by default, I think that choice should be enough for the rest to configure itself to be usable. What are your thoughts on simplifying some of your PR to make the module "just work" based on the user's choice of package?

That's also why I still think the approach from #328890 requires probably lower maintenance of the NixOS module code (major concern for #328890) than the approach here.

Did you mean to put the same PR for both of those links? Assuming so -- that's interesting, I still worry that the complexity of that approach would make it far more difficult (for me! though perhaps not for you or hypothetical maintainers) to debug / maintain when something eventually goes wrong. Even if it isn't the ultimate cause of a bug, when a bug arises this code should be considered as part of the machinery that is under examination -- and I don't think I could do that well. I am reasonably comfortable with nix and rust, which is why I thought I could help on the respective teams, but I never learned the C family!

That said, your explanatory comments in the code are wonderful and give me a fighting chance at understanding the approach you've taken.

Yes, but they're running espanso launcher, not espanso worker. I just wanted to point out that this is another indicator that the launcher does more than simply restarting espanso on failures.

Correct -- and this is part of my point. Both NixOS and Home-Manager modules are currently using espanso daemon -- which is also undocumented but seems to have been working well so far. The documented / blessed path for Linux is to imperatively run espanso service register and then espanso start thereafter -- it doesn't mention launcher AFAICT. It even does runtime checks to make sure the systemd file points to the expected binary and expects to find it at /usr/local/bin/espanso. For MacOS, the only documented install processes are downloading the .app to Applications or homebrew, and espanso installs its own plist to manage the service, which also uses espanso launcher (not espanso start).

So whichever way we go, just by using nix, I think we're already pretty far off the well documented path.

It seems that @KenMacD has been using the espanso worker approach for months with generally satisfactory results while the rest of us wayland suckers languish :)

[…] Also, regarding your PR -- IMO users installing espanso with package = pkgs.espanso-wayland should -- with documentation -- be opting into the wrapper by default. If espanso cannot work on wayland without these capabilities, and we're not going to provide a service that runs as root by default, I think that choice should be enough for the rest to configure itself to be usable. What are your thoughts on simplifying some of your PR to make the module "just work" based on the user's choice of package?

I just did update #328890. It is now its own small module programs.espanso.capdacoverride that, if enabled (right now the default, but eventually it should default to disabled and be activated either manually or by the NixOS Espanso module) makes sure that everything works out of the box. To do so it creates the security wrapper and overlays pkgs.espanso-wayland with the capability-enabled version. I'm not sure whether automatic overlays by modules are a good idea, but at least makes sure that pkgs.espanso-wayland just works, no matter where it is used.

(BTW: I also just tested myself that running espanso worker directly does not pick up configuration changes, while that works flawlessly with #328890).

That's also why I still think the approach from #328890 requires probably lower maintenance of the NixOS module code (major concern for #328890) than the approach here.

Did you mean to put the same PR for both of those links? Assuming so -- that's interesting, I still worry that the complexity of that approach would make it far more difficult (for me! though perhaps not for you or hypothetical maintainers) to debug / maintain when something eventually goes wrong. Even if it isn't the ultimate cause of a bug, when a bug arises this code should be considered as part of the machinery that is under examination -- and I don't think I could do that well. I am reasonably comfortable with nix and rust, which is why I thought I could help on the respective teams, but I never learned the C family!

That said, your explanatory comments in the code are wonderful and give me a fighting chance at understanding the approach you've taken.

I absolutely get your reservation, and if you don't feel those changes are worth the hassle, that's fine :). If it helps, I'd be willing to maintain that part if wanted. BUT I should say that I have never written a single line of Rust code, so not sure how helpful I can be with issues related to Espanso internals.

But yes, I think it's lower maintenance (I meant PR #328890 in both cases). I think it is pretty orthogonal to any changes to the espanso package and module. I cannot see many upstream changes that would cause hiccups with the wrapper; the most likely breaking change would be that Espanso no longer uses the libc-based fork, but then the wrapper just behaves as the original. Also, one can easily disable the wrapper completely via programs.espanso.capdacoverride.enable = false; with this, there is actually no difference at all to the status quo (so one can easily check whether the wrapper is causing problems or not).

Note that the way #328890 is implemented now does neither touch the Espanso package definition nor the Espanso module code directly. The way I wrap the Espanso package in nixos/modules/programs/espanso-capdacoverride/espanso-capdacoverride.nix is quite orthogonal to the original package definition. And by overlaying pkgs.espanso-wayland (again, not sure this is a good approach) it requires no changes to the espanso NixOS module at all and immediately works for the corresponding home-manager module (or any other place using pkgs.espanso-wayland) without any changes.

Yes, but they're running espanso launcher, not espanso worker. I just wanted to point out that this is another indicator that the launcher does more than simply restarting espanso on failures.

Correct -- and this is part of my point. Both NixOS and Home-Manager modules are currently using espanso daemon -- which is also undocumented but seems to have been working well so far. […]

So whichever way we go, just by using nix, I think we're already pretty far off the well documented path. […]

Ah, I wasn't aware that daemon and launcher are also not documented. Very good point! That definitely eases my concerns with respect to the stability of worker. Still, we would have at least to figure out what problems may occur (like not reloading the configuration) without the monitoring code from launcher and whether one can fix them somehow (or live with them).

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/espanso-daemon-problem/35309/27

Closing in favor of #328890